feat: Add Time configtype #1905

rhino1998 · 2024-09-23T16:36:22Z

Summary

This adds a new Time type to be used in plugin specs that can represent either fixed times or relative times. A future PR should extend the duration parsing to handle days, etc.

kind: source
spec:
  name: "orb"
  registry: "grpc"
  path: "localhost:7777"
  version: "v1.0.0"
  tables:
    ["*"]
  destinations:
    - "sqlite"
  spec:
    api_key: "${CQ_ORB_API_KEY}"
    timeframe_start: 10 days ago
    timeframe_end: "2024-09-25T03:52:40.14157935-04:00"

Here timeframe_start and timeframe_end are both configtype.Time types. Using mixed relative/fixed timestamps for timeframes isn't really useful here as the relative one will be relative to the run-time of the sync but in this example having

timeframe_start: 10 days ago
timeframe_end: now

or even omitting timeframe_end entirely is more useful

timeframe_start: 10 days ago

The plugin is free to define default like so

type Spec struct {
	// TimeframeStart is the beginning of the timeframe in which to fetch cost data.
	TimeframeStart configtype.Time `json:"timeframe_start"`

	// TimeframeEnd is the beginning of the timeframe in which to fetch cost data.
	TimeframeEnd configtype.Time `json:"timeframe_end"`

	// ...
}


func (s *Spec) SetDefaults() {
	if s.TimeframeStart.IsZero() {
		s.TimeframeStart = configtype.NewRelativeTime(-1 * 365 * 24 * time.Hour)
	}

	if s.TimeframeEnd.IsZero() {
		s.TimeframeEnd = configtype.NewRelativeTime(0)
	}
}

erezrokah

Hi @rhino1998, nice 🚀 Can you give an example how this will be used in a plugin and in a spec file?

rhino1998 · 2024-09-23T17:27:30Z

Added some more context. Should configtype.Time.Time take a time.Time instead of a func() time.Time?

erezrokah

I like the idea of having this kind of type as an SDK helper.

A few comments:

If the purpose is to make it easier to configure relative times, I would use https://github.com/tj/go-naturaldate/tree/master instead of a wrapper over duration
I would hide the IsRelative and NewRelativeTime and do NewTime(t string) that accepts either an RFC3339 or a natural date string. I don't see a reason consumers need to know the internal representation, also I don't think we even need typ timeType, as you can have NewTime(t string, base time.Time) and convert the string on creation.

erezrokah · 2024-09-24T08:10:24Z

configtype/time.go

+
+	var err error
+	switch {
+	case timeRFC3339Regexp.MatchString(s):


Instead of using regular expressions can't we try to parse with each format, then return an error if all formats failed to parse?

Yeah, although we still need regexes for the jsonschema definitions

Also, the regexes let us give a more specific error, otherwise we'd always error with a duration parsing error (or whatever the last parsing case is).

hermanschaaf · 2024-09-24T09:35:25Z

If the purpose is to make it easier to configure relative times, I would use https://github.com/tj/go-naturaldate/tree/master instead of a wrapper over duration

I would push back on this; the library was last updated 4 years ago, and supports much more than we need to support. I think we would be better off supporting a small, well-defined number of formats.

Also I think the purpose here is not to make it easier, it's to make it possible at all (in Cloud), in a standardized way.

erezrokah · 2024-09-24T09:50:36Z

the library was last updated 4 years ago

I agree it hasn't change in a long time, but it doesn't have any other dependencies (other than the ones used for testing), and the author is https://github.com/tj (https://x.com/tjholowaychuk), which has a bunch of other popular repositories like https://github.com/tj/commander.js and https://github.com/koajs/koa

well-defined number of formats.

Agree it does more than we need, but timeframe_start: -1000h seems more machine friendly than human friendly to me

Also I think the purpose here is not to make it easier, it's to make it possible at all (in Cloud), in a standardized way.

Can you add more context on the Cloud use case? We want the backend to specify a duration via spec generated via backend code in Go? If so is it possible to convert the duration to time before?

hermanschaaf · 2024-09-24T11:27:40Z

Can you add more context on the Cloud use case? We want the backend to specify a duration via spec generated via backend code in Go? If so is it possible to convert the duration to time before?

We want users to be able to specify a relative time, such as "30 days ago", which will be stored as part of the spec. Every time the sync runs, this should be converted to a timestamp relative to the current time. This way, we don't need to involve the backend to do string interpolation on the spec, and things remain relatively simple.

Right now, what happens in many plugins is they allow only a fixed timestamp to be specified, such as 2024-05-01T00:00:00Z, or a default value, often relative (30 days ago or now). This doesn't lend us the flexibility we desire for scheduled syncs in Cloud

hermanschaaf · 2024-09-24T11:30:36Z

Agree it does more than we need, but timeframe_start: -1000h seems more machine friendly than human friendly to me

I agree with this.

I'm also okay with using that library, but if we do, let's not support everything that the library supports, but instead limit it to a defined list of supported formats that we can control.

I would maybe say something like:

now
x seconds [ago|from now]
x minutes [ago|from now]
x hours [ago|from now]
x days [ago|from now]

would be a good enough start (we can always add more if requested)

erezrokah · 2024-09-24T11:33:43Z

I would maybe say something like:

now

x seconds [ago|from now]

x minutes [ago|from now]

x hours [ago|from now]

x days [ago|from now]

would be a good enough start (we can always add more if requested)

Sounds like a plan to me

hermanschaaf · 2024-09-24T12:09:54Z

I forgot that in addition to relative times, we'd also want to support fixed timestamps of course, probably using RFC 3339. And also some support for plain dates, e.g. 2024-05-10, for plugins that need it.

feat: Add Time configtype

7bc83b0

rhino1998 requested a review from a team as a code owner September 23, 2024 16:36

rhino1998 requested review from marianogappa and hermanschaaf September 23, 2024 16:36

github-actions bot added the feat label Sep 23, 2024

cleanup

6c6e1a6

erezrokah reviewed Sep 23, 2024

View reviewed changes

rhino1998 added 3 commits September 23, 2024 12:41

return zero time on nil Time

34a31d6

add time type and fix zero value

93f2449

function typing

852db5a

github-actions bot added feat and removed feat labels Sep 23, 2024

zero handling

70a90a4

github-actions bot added feat and removed feat labels Sep 23, 2024

use time.Time instead of func() time.Time

3111bb1

github-actions bot added feat and removed feat labels Sep 23, 2024

erezrokah requested changes Sep 24, 2024

View reviewed changes

erezrokah reviewed Sep 24, 2024

View reviewed changes

add ParseTime

3c5a1ac

rhino1998 added 2 commits September 24, 2024 10:59

new duration parsing

26eea6b

fix tests and add now

274a303

github-actions bot added feat and removed feat labels Sep 24, 2024

rhino1998 added 4 commits September 24, 2024 12:40

complex literals and better marshalling

d5ce087

better test

99244f8

lints

1eadc7f

lints

35bf99e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add Time configtype #1905

feat: Add Time configtype #1905

rhino1998 commented Sep 23, 2024 •

edited

Loading

erezrokah left a comment

rhino1998 commented Sep 23, 2024

erezrokah left a comment

erezrokah Sep 24, 2024

rhino1998 Sep 24, 2024

rhino1998 Sep 24, 2024

hermanschaaf commented Sep 24, 2024 •

edited

Loading

erezrokah commented Sep 24, 2024 •

edited

Loading

hermanschaaf commented Sep 24, 2024

hermanschaaf commented Sep 24, 2024

erezrokah commented Sep 24, 2024

hermanschaaf commented Sep 24, 2024

feat: Add Time configtype #1905

Are you sure you want to change the base?

feat: Add Time configtype #1905

Conversation

rhino1998 commented Sep 23, 2024 • edited Loading

Summary

erezrokah left a comment

Choose a reason for hiding this comment

rhino1998 commented Sep 23, 2024

erezrokah left a comment

Choose a reason for hiding this comment

erezrokah Sep 24, 2024

Choose a reason for hiding this comment

rhino1998 Sep 24, 2024

Choose a reason for hiding this comment

rhino1998 Sep 24, 2024

Choose a reason for hiding this comment

hermanschaaf commented Sep 24, 2024 • edited Loading

erezrokah commented Sep 24, 2024 • edited Loading

hermanschaaf commented Sep 24, 2024

hermanschaaf commented Sep 24, 2024

erezrokah commented Sep 24, 2024

hermanschaaf commented Sep 24, 2024

rhino1998 commented Sep 23, 2024 •

edited

Loading

hermanschaaf commented Sep 24, 2024 •

edited

Loading

erezrokah commented Sep 24, 2024 •

edited

Loading